fix: redact project credentials on egress - #1702

sequenceDiagram
  participant Desktop
  participant Core
  participant ShareService
  participant Collaboration
  participant PythonAPI
  Desktop->>Core: redactProjectCredentials(project)
  Core-->>Desktop: sanitized project and redaction count
  Desktop->>ShareService: upload serialized sanitized project
  Desktop->>Collaboration: send sanitized snapshot
  PythonAPI->>Core: redact project for export
  Core-->>PythonAPI: sanitized project

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

🔍 Cloudflare PR preview

🔍 GitHub Pages PR preview

Code review

Bugs

• apps/geolibre-desktop/src/hooks/useCollaboration.ts:79 (inline comment posted) — sendSnapshot now builds/stores lastContentRef.current from the redacted buildProjectEgressSnapshot, but applyRemoteSnapshot (line 108, untouched by this PR) still baselines it from the un-redacted buildProjectSnapshot. Whenever the project holds any credentials, the dedup check content === lastContentRef.current will mismatch right after any remote snapshot is applied, causing one spurious extra snapshot broadcast. Confidence: medium-high.
• apps/geolibre-desktop/src/hooks/useProjectFileActions.ts:845 (inline comment posted) — passes redacted.redactedPaths.length (deduplicated paths) instead of redacted.redactedCount (actual field count) into the "Strip credentials?" prompt, understating the count shown to the user whenever multiple secrets share one path (e.g. several environment variables). ShareProjectDialog/TopToolbar.tsx correctly use redactedCount for the equivalent message. Confidence: medium.

Security

• packages/core/src/credentials.ts:35-56 (inline comment posted) — URL_CREDENTIAL_PARAMS is a fixed denylist; it covers subscription-key/subscriptionkey but not the equally plausible api-key (only api_key/apikey), and misses other common names (auth, secret, sasToken, bearer, …). The mirrored Python list has the same gap. docs/project-format.md states the invariant in absolute terms ("removes credential-bearing fields and authentication parameters in URLs"), which could give false confidence for an unlisted provider param. Confidence: medium.

Quality

• packages/core/src/credentials.ts:3-22 (inline comment posted) — PROJECT_CREDENTIAL_FIELDS's docstring frames it as "the schema-level decision point," but only layerConfiguration actually drives redaction (SENSITIVE_KEYS); preferences/pluginState entries are enforced by separate hardcoded logic further down, so editing the registry alone wouldn't change behavior. Confidence: low-medium.
• apps/geolibre-desktop/src/hooks/useEmbedBridge.ts:39 — the "Trust model" doc comment ("the embedding host is... receives the entire project state") is now stale since postState/applyLoad send the redacted buildProjectEgressSnapshot, not the entire project. Not part of the diff hunk so no inline comment was posted. Confidence: low.
• python/src/geolibre/project.py:86 (_redact_config) — unlike the JS redactConfigurationValue, which fails closed at MAX_REDACT_DEPTH = 12, the Python version has no recursion guard, so a pathologically deep/nested layer.source/metadata dict could raise RecursionError in to_html()/save_project() instead of degrading gracefully. Low real-world likelihood; not part of the diff hunk so no inline comment was posted. Confidence: low.

CLAUDE.md

• This PR introduces a new pair of mirrored magic constants (JS credentials.ts's field/param lists vs. Python project.py's _CREDENTIAL_FIELD_NAMES/_CREDENTIAL_URL_PARAMS) that must be kept in sync by hand, matching the exact pattern CLAUDE.md's Conventions section calls out repeatedly for other mirrored constants (MAX_VECTOR_PMTILES_ZOOM, MAX_VECTOR_BYTES, MAP_PANEL_SELECTOR, etc.), each with an explicit "whenever X is bumped, re-check Y" note and a regression test. This new pair has no such note and no cross-language drift test. Consider adding a short convention entry so a future edit to one list isn't silently missed in the other. Confidence: low-medium (a reasonable suggestion consistent with existing project convention, not a stated rule this PR violates).

Performance — no notable issues found; the redaction pass runs once per egress event on already-small project objects.

Code review

Bugs

• packages/core/src/credentials.ts (redactProjectCredentials, ~L209-217) and the parallel python/src/geolibre/project.py (redact_credentials, ~L122-127): only preferences.geocoding.apiKeys is cleared — forwardEndpoint/reverseEndpoint, the free-text custom geocoding endpoint URLs a user can set in Settings, are passed through untouched. Several built-in providers append the API key directly onto whichever endpoint is configured (packages/core/src/geocoding.ts), so a self-hosted/proxied endpoint with a credential embedded in its URL (e.g. ?key=SECRET) survives Share, HTML export, collaboration/embed snapshots, and Python egress — the exact class of leak this PR is meant to close. Confidence: high.

Quality

• python/src/geolibre/project.py _redact_config (~L100-114) has no depth guard, unlike the JS redactConfigurationValue, which fails closed at MAX_REDACT_DEPTH = 12 (and is covered by a dedicated test). The two implementations are meant to enforce one shared invariant but now diverge in worst-case behavior; deeply nested config could also raise RecursionError in Python instead of degrading safely. Confidence: medium.
• packages/core/src/credentials.ts L156: the GeoJSON early-return (isGeoJsonPayload) returns the original object by reference instead of cloning, unlike the Python port which does copy.deepcopy. Harmless today since every caller serializes the result immediately, but it undercuts the "detached project" guarantee documented on the function. Confidence: low.
• apps/geolibre-desktop/src/hooks/useProjectFileActions.ts (~L947-951, outside the diff hunk so not inline-commentable): the comment above buildEmbeddedProject/buildProjectHtml in handleExportHtml still says stripping happens "here," but redaction now happens inside buildProjectHtml via redactCredentials and covers all credential fields, not just env vars. Stale comment, no functional impact. Confidence: low.

No issues found in the i18n translation additions, the double-redaction in TopToolbar.tsx/share-geolibre.ts (redundant but idempotent and intentional defense-in-depth), or the new test coverage, which is thorough and correctly validates the depth-cutoff and broker-reference-preservation behavior that does exist.

All inline comments are posted. Here is the final review summary.

Code review

Bugs

• None found. The core redaction logic (redactUrlCredentials/redactConfigurationValue in packages/core/src/credentials.ts and their Python mirrors in python/src/geolibre/project.py) traced correctly against its own test suite, including the userinfo-stripping, query/fragment credential-parameter filtering, GeoJSON short-circuiting, and depth-capped fail-closed behavior. Confidence: high.

Security

• python/src/geolibre/project.py's redact_credentials doesn't redact a layer's sourcePath, unlike the TS redactProjectCredentials, which does. Low practical impact since sourcePath is documented as a local filesystem path, but it's a parity gap between the two egress implementations. Confidence: medium.
• Layer Library Export (packages/core/src/layer-library.ts, EXPORT_REDACTED_SOURCE_KEYS) — an egress path the code's own comment frames as "a far more direct exposure path than handing someone a whole project file" — still only strips the requestHeaders key, not the broader apiKey/token/password/clientSecret/connectionString set this PR introduces for the main project egress boundary. Pre-existing code, only its import was touched here, so may be intentionally out of scope. Confidence: medium.
• redactProjectCredentials/redact_credentials don't cover project.models (Processing pipelines) or project.processingHistory, whose parameters bags could theoretically carry a credential for some future tool. This is explicitly acknowledged in a code comment, so it reads as a deliberate scope boundary rather than an oversight — flagged mainly because the new docs describe the invariant in fairly absolute terms. Confidence: low.

Performance

• No issues found. The redaction pass short-circuits on GeoJSON payloads via structuredClone rather than recursively scanning feature properties, keeping the added per-snapshot cost (now run on every collaboration sync and embed post) roughly linear in configuration size rather than data size. Confidence: high.

Quality

• runSaveProject (useProjectFileActions.ts) now shows the "Strip credentials?" prompt on every local save whenever any credential-bearing field exists (previously scoped to populated env vars only), with no "remember my choice." This broadens a per-save interruption to any project with a routinely authenticated layer. Likely intentional per the PR's stated goal, but worth confirming the frequency is acceptable. Confidence: medium.
• askStripEnvVars/EnvStripPrompt naming wasn't updated even though its scope broadened from env vars to all credential types — a minor readability nit. Confidence: low.
• A comment in useProjectFileActions.ts's HTML-export flow ("...and strip env vars") is now stale: the explicit env-var-stripping code it describes was removed in this PR and redaction now happens inside buildProjectHtml. Confidence: medium.

CLAUDE.md

• No violations found. uv.lock/build-catalog/mirror-constant conventions aren't implicated by this change, and new user-facing strings correctly go through t() with catalogs updated across all locales.

Code review

Bugs

• apps/geolibre-desktop/src/hooks/useEmbedBridge.ts:66 — The embed bridge now redacts every geolibre:state snapshot before posting it, but this same bridge is the sync channel for the Jupyter/anywidget Python widget, which its own surrounding docs (this file, embedHost.ts, _frontend.js, geolibre.py's Map docstring) describe as fully trusted, full-fidelity, two-way sync. Any store change (even a pan) now silently strips credentials from Python's self.project trait, which defeats the new keep_credentials=True option this PR adds to to_project()/save_project(). Medium-high confidence (traced through code, not runtime-verified).

Quality

• Same location — the "fully trusted"/"broadcasts full project state" docstrings in useEmbedBridge.ts and embedHost.ts are now stale relative to the new redaction behavior, regardless of whether the change itself is intentional.
• packages/core/src/credentials.ts:143-150 — the URL-credential sweep now runs over every string in layer.metadata/project.metadata (new territory vs. the pre-PR scope), not just URL-shaped fields, so free-form text that happens to contain a ?token=...-like substring could be silently mangled. Low-medium confidence, narrow edge case.
• packages/core/src/layer-library.ts:710 — the Layer Library export sweep and the new central credentials.ts sweep now share a field registry but diverge on depth-limit failure mode (fail-open at depth 6 here vs. fail-closed at depth 12 in credentials.ts), which is inconsistent with the PR's stated single-invariant goal, though this specific behavior predates the PR. Low-medium confidence.

Security / Performance / CLAUDE.md

• No additional findings. The core redaction logic (URL userinfo/query/fragment stripping, recursive field scrubbing, depth-limited fail-closed traversal, plugin-settings wholesale removal, Python/JS parity) is well-tested and consistent across Share, HTML export, collaboration, and Python egress paths; i18n locale updates look complete and consistent across all listed languages; no CLAUDE.md convention violations (translations use t(), no node_modules edits, no lockfile-affecting dependency changes) were observed.

All inline comments are posted. Here is the final summary.

Code review

Bugs

• None found. The redaction pass is non-mutating (verified by the "detached inline GeoJSON" test and by tracing that redactConfigurationValue/redactProjectCredentials always build new objects/arrays), and the redactedCount accounting checks out against the test fixture (9 distinct redaction events, no double-counting despite the [...new Set(redactedPaths)] dedup on paths only).

Security

• packages/core/src/credentials.ts: the object-key credential registry (PROJECT_CREDENTIAL_FIELDS.layerConfiguration / SENSITIVE_KEYS) is narrower than the URL-parameter registry (URL_CREDENTIAL_PARAMS) — names like secret, bearer, sasToken, authKey, subscriptionKey, pwd, sig are treated as credentials in a URL query string but not when they appear as a plain JSON field on a layer's source. Medium confidence.
• apps/geolibre-desktop/src/hooks/useEmbedBridge.ts: the new trustedWidget flag that decides whether the embed bridge returns the full or redacted project snapshot is read verbatim off the inbound message from window.parent — i.e. self-declared by the very host it's meant to gate. For the ?embed=1 path, which the file's own docstring already documents as treating "whatever the framing parent is" as fully trusted, any such host can simply set trustedWidget: true to get the unredacted snapshot back, so the flag protects only well-behaved hosts, not adversarial ones. Medium confidence — this may be intentional (the widget genuinely needs credentials round-tripped), but it isn't documented as a caveat.
• packages/core/src/layer-library.ts (redactSourceValue): fails open past its MAX_REDACT_DEPTH (returns the value unchanged), the opposite of the fail-closed design the PR's own new credentials.ts module uses at its depth cap, with an explicit comment explaining why fail-closed is correct. The PR touches this exact function but doesn't reconcile the inconsistency. Medium confidence.

Performance

• No concerns; the redaction traversal is bounded by depth caps and runs only on egress paths (share/export/collab/embed), not on the hot render path.

Quality

• useEmbedBridge.ts's docstring ("the embedding host is fully trusted and receives the entire project state") wasn't updated to reflect the new conditional (trustedWidget-gated) default-redact behavior — see inline comment. Medium-high confidence.
• useProjectFileActions.ts: askStripEnvVars/envStripPrompt naming is stale now that the prompt covers all credential types, not just environment variables (the i18n strings were already generalized). Low severity, cosmetic.
• docs/python.md isn't updated to mention the new keep_credentials parameter on Map.to_project()/Map.save_project() or the new default redaction behavior of to_html() — a user-facing API behavior change with no corresponding doc update. Low-medium confidence this matters given docs/project-format.md does cover the JS-side invariant.

CLAUDE.md

• No violations found. i18n strings were added consistently across all locale files with en.json as source of truth; no MapLibre control CSS, WASM catalog, or other documented mirror invariants were touched.

Both inline comments posted successfully. Now finalizing the summary.

Code review

Bugs

• None found beyond the redaction gap noted under Security — the store/UI wiring (TopToolbar, ShareProjectDialog, useEmbedBridge, useCollaboration) is internally consistent and covered by matching tests. Confidence: medium (reviewed the full data flow but did not execute the test suite).

Security

• python/src/geolibre/project.py's _CREDENTIAL_FIELD_NAMES (object-key redaction for layer.source/metadata/sourcePath) is missing secret, bearer, auth, authKey, sasToken, subscriptionKey, signature, pwd — all present in the TS PROJECT_CREDENTIAL_FIELDS.layerConfiguration registry and explicitly covered by a JS test ("removes credential-named configuration fields in every spelling"). These names are only caught by Python's URL-parameter set, not its object-key set, so to_project()/save_project()/to_html() can ship a credential shaped as e.g. {"bearer": "..."} or {"pwd": "..."} in a layer source that the JS-side redaction would strip. Posted inline with a suggested fix. Confidence: high.
• The trustedWidget self-declared flag in useEmbedBridge.ts is a known, clearly documented trust narrowing rather than a boundary — the PR's own comments are candid that any framing page can set it. Not a new gap, just worth confirming reviewers are comfortable with that residual risk. Confidence: low (already acknowledged by the authors).

Performance

• redactProjectCredentials is invoked on every debounced store change for both collaboration (useCollaboration.ts) and embed (useEmbedBridge.ts) snapshots (every 250ms during active edits, including pure camera pans that don't touch layers), and does a structuredClone of any embedded GeoJSON payload plus a full recursive walk of layer source/metadata each time. Since the existing code already serializes the full project on every such change, this is incremental rather than new cost, but for projects with large embedded vector layers it adds a repeated full deep-clone that wasn't there before. Confidence: low-medium.
• The Share flow redacts twice (once in TopToolbar.tsx's getProject, again inside uploadProjectToShare) — harmless correctness-wise (redaction is idempotent, tested) but doubles the traversal/clone cost for large projects. Confidence: low (intentional defense-in-depth, minor cost).

Quality

• Stale comment in useProjectFileActions.ts (around the HTML-export handler) still describes stripping "env vars" locally, but that responsibility moved into buildProjectHtml/redactCredentials. Posted inline. Confidence: low.
• Widening the local Save/Save-As "strip credentials?" prompt trigger from "any environment variables set" to "any redactable field present" means the prompt will now fire on ordinary local saves whenever a layer URL contains a key=-style parameter (common for many tile providers' non-secret app IDs), not just when real secrets are configured — a plausible UX regression versus the previous, narrower trigger. This appears to be an intentional, documented trade-off (docs/project-format.md), so flagging for awareness rather than as a defect. Confidence: low-medium.

CLAUDE.md

• No violations found: i18n strings use t() and are added to en.json plus all locale catalogs; no direct MapLibre mutation; no new external tile/plugin hosts introduced. Confidence: medium.

Code review

Bugs

• None found. The redaction pass in packages/core/src/credentials.ts (and its Python mirror in python/src/geolibre/project.py) is carefully constructed — depth-capped fail-closed recursion, GeoJSON payloads correctly excluded from the sweep to avoid corrupting feature data, URL/userinfo/query/fragment stripping handled consistently, and idempotency verified by tests. I traced the full egress surface (Share, HTML export, embed bridge, collaboration snapshots, Python to_project/save_project/to_html) and each path is wired through the central redaction helpers with no missed call sites.

Security

• (Medium-low confidence) layer.connection (specifically connection.lastError, a free-form error-message string) is not covered by the credential redaction sweep in either the JS or Python implementation — only source, metadata, and sourcePath are swept per layer. No currently-shipped code path leaks a credential through it (built-in refresh errors are generic strings), but it's a blind spot relative to the module's stated "no-secret guarantee" if a future error path or plugin ever embeds a URL with a token in that field. Posted inline on packages/core/src/credentials.ts.
• (Already mitigated, informational) useEmbedBridge.ts's trustedWidget flag is self-declared by the framing host and is explicitly documented as a fidelity switch rather than a security boundary — this is a deliberate, well-reasoned tradeoff by the author, not a defect.

Performance

• No concerns. Redaction runs synchronously over already-in-memory project objects at export time; depth caps (12 in credentials.ts, 6 in layer-library.ts) bound worst-case recursion.

Quality

• Minor: packages/core/src/credentials.ts and packages/core/src/layer-library.ts each define their own MAX_REDACT_DEPTH constant (12 vs. 6) rather than sharing one, and the comment in layer-library.ts says its fail-closed behavior is "matching redactConfigurationValue in credentials.ts," which could be misread as matching the depth value too. Cosmetic only.
• The JS and Python credential registries (PROJECT_CREDENTIAL_FIELDS.layerConfiguration / _CREDENTIAL_FIELD_NAMES) are duplicated by hand with a comment asking maintainers to keep them in sync manually — a drift risk, but reasonable given the two runtimes can't share a module, and it's explicitly called out.

CLAUDE.md

• No violations found. New i18n strings use t() and are present in all locale catalogs with consistent {{count}} placeholders; no MapLibre control CSS, no Tauri CSP, no Whitebox/menu-catalog, or other guarded-invariant files were touched.